async-hooks,net: ensure asyncId=null if no handle by baudehlo · Pull Request #13938 · nodejs/node

baudehlo · 2017-06-26T22:06:27Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
- Note this doesn't pass, but that's because master was failing before. The commit I added passes.
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

async-hooks, net

baudehlo · 2017-06-26T22:06:56Z

addaleax

Can you make a few edits? Also, it would be perfect if you could include Fixes: https://github.com/nodejs/node/issues/13548 in the commit message itself so that the issue will automatically be closed when this is merged.

Thank you for doing this!

@nodejs/async_hooks

addaleax · 2017-06-26T22:07:43Z

test/async-hooks/test-get-connections.js

Can you indent using 2 spaces? I think our linter complains otherwise.

addaleax · 2017-06-26T22:07:53Z

test/async-hooks/test-get-connections.js

Can you const common = require('../common'); here?

addaleax · 2017-06-26T22:08:57Z

test/async-hooks/test-get-connections.js

You can just call server.getConnections(common.mustCall()) or server.getConnections(common.mustCall(fn)), that’s a bit shorter (and actually makes sure that the callback gets executed) :)

addaleax · 2017-06-26T22:09:23Z

test/async-hooks/test-get-connections.js

You don’t need the try/catch, if an error is thrown the test is going to fail anyway because the process crashes with an uncaught exception

OK, would you prefer a comment to that effect then? I was trying to write the test showing the context of what happened before the change.

refack

LGTM % previous comments (to get linter passing)

baudehlo · 2017-06-27T01:19:45Z

Will address the edits. I think adding the Fixes line in a comment works with github too fwiw.

refack · 2017-06-27T01:49:34Z

test/async-hooks/test-get-connections.js

The comment is good 👍
Style nit: line comments (//) are prefered.
And replace See: with Ref:

refack · 2017-06-27T01:50:15Z

test/async-hooks/test-get-connections.js

unused assert will fail linter.

refack · 2017-06-27T01:52:00Z

CI: https://ci.nodejs.org/job/node-test-commit/10779/
Lint fail:

not ok 3 - /usr/home/iojs/build/workspace/node-test-linter/test/async-hooks/test-get-connections.js
  ---
  message: '''assert'' is assigned a value but never used.'
  severity: error
  data:
    line: 4
    column: 7
    ruleId: no-unused-vars
  messages:
    - message: Trailing spaces not allowed.
      severity: error
      data:
        line: 10
        column: 1
        ruleId: no-trailing-spaces
  ...

mscdex · 2017-06-27T06:03:46Z

I guess this is the kind of change needed for http.Agent as well?

AndreasMadsen · 2017-06-27T07:03:58Z

test/async-hooks/test-get-connections.js

Hi, please rename the file to test-net-get-connections.js.

AndreasMadsen · 2017-07-03T08:28:01Z

CI: https://ci.nodejs.org/job/node-test-pull-request/8942/

@baudehlo will you rename the file and add Fixes: https://github.com/nodejs/node/issues/13548 to the commit message. If not, I can do it myself.

baudehlo · 2017-07-03T11:47:26Z

Just moved house and have no internet until Friday. So could you do it?

…

On Jul 3, 2017, at 4:28 AM, Andreas Madsen ***@***.***> wrote: CI: https://ci.nodejs.org/job/node-test-pull-request/8942/ @baudehlo will you rename the file. If not, I can do it myself. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

If the .listen() hasn't been called on the server, there is no handle object. In this case use null as the triggerAsyncId. Fixes: nodejs#13548

AndreasMadsen · 2017-07-03T12:01:13Z

Sure.

CI: https://ci.nodejs.org/job/node-test-pull-request/8951/

AndreasMadsen

LGTM

trevnorris

Thank you

AndreasMadsen · 2017-07-05T13:53:49Z

landed in aa8655a

tniessen · 2017-07-05T13:58:33Z

I think it should have been async_hooks in the commit title. not async-hooks. Just for the future.

AndreasMadsen · 2017-07-05T14:07:59Z

@tniessen sorry about that.

If the .listen() hasn't been called on the server, there is no handle object. In this case use null as the triggerAsyncId. Fixes: #13548 PR-URL: #13938 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Andreas Madsen <amwebdk@gmail.com>

nodejs-github-bot added the net Issues and PRs related to the net subsystem. label Jun 26, 2017

addaleax reviewed Jun 26, 2017

View reviewed changes

addaleax added the async_hooks Issues and PRs related to the async hooks subsystem. label Jun 26, 2017

refack approved these changes Jun 26, 2017

View reviewed changes

jasnell approved these changes Jun 26, 2017

View reviewed changes

refack approved these changes Jun 27, 2017

View reviewed changes

refack reviewed Jun 27, 2017

View reviewed changes

test/async-hooks/test-get-connections.js Outdated

Copy link

Contributor

refack Jun 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unused assert will fail linter.

AndreasMadsen reviewed Jun 27, 2017

View reviewed changes

test/async-hooks/test-get-connections.js Outdated

Copy link

Member

AndreasMadsen Jun 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, please rename the file to test-net-get-connections.js.

Matt Sergeant and others added 4 commits July 3, 2017 13:56

async-hooks,net: ensure asyncId=null if no handle

b68cc5c

If the .listen() hasn't been called on the server, there is no handle object. In this case use null as the triggerAsyncId. Fixes: nodejs#13548

[squash] Address PR requests

bbdc7c1

[squash] More cleanups from PR comments

fc5a7fc

[squash] rename test

6ccf034

AndreasMadsen force-pushed the issue-13548 branch from a968e81 to 6ccf034 Compare July 3, 2017 12:00

AndreasMadsen added dont-land-on-v4.x labels Jul 3, 2017

AndreasMadsen approved these changes Jul 3, 2017

View reviewed changes

refack approved these changes Jul 3, 2017

View reviewed changes

trevnorris approved these changes Jul 3, 2017

View reviewed changes

AndreasMadsen closed this Jul 5, 2017

addaleax mentioned this pull request Jul 11, 2017

v8.2.0 proposal #13744

Merged

msimerson mentioned this pull request Jul 25, 2017

Haraka doesn't start. haraka/Haraka#2005

Closed

Uh oh!

Comments

Conversation

baudehlo commented Jun 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

baudehlo commented Jun 26, 2017

Uh oh!

addaleax left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

baudehlo commented Jun 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack commented Jun 27, 2017

Uh oh!

mscdex commented Jun 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndreasMadsen commented Jul 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

baudehlo commented Jul 3, 2017 via email

Uh oh!

AndreasMadsen commented Jul 3, 2017

Uh oh!

AndreasMadsen left a comment

Choose a reason for hiding this comment

Uh oh!

trevnorris left a comment

Choose a reason for hiding this comment

Uh oh!

AndreasMadsen commented Jul 5, 2017

Uh oh!

tniessen commented Jul 5, 2017

Uh oh!

AndreasMadsen commented Jul 5, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

baudehlo commented Jun 26, 2017 •

edited

Loading

addaleax left a comment •

edited

Loading

AndreasMadsen commented Jul 3, 2017 •

edited

Loading